Skip to content
This repository was archived by the owner on Oct 25, 2021. It is now read-only.

Andropov_Vsevolod#29

Open
heptaber wants to merge 68 commits intomasterfrom
feature/AndropovVsevolod
Open

Andropov_Vsevolod#29
heptaber wants to merge 68 commits intomasterfrom
feature/AndropovVsevolod

Conversation

@heptaber
Copy link
Collaborator

@heptaber heptaber commented Jul 9, 2021

No description provided.

@heptaber heptaber added the readyForReview Sign for Artem to take a look label Jul 10, 2021
@heptaber heptaber requested a review from NikolaevArtem July 10, 2021 12:10
@heptaber heptaber self-assigned this Jul 10, 2021
@NikolaevArtem NikolaevArtem added bug Code fix needed and removed readyForReview Sign for Artem to take a look labels Jul 11, 2021
@NikolaevArtem NikolaevArtem changed the title feature/AndropovVsevolod Andropov_Vsevolod Jul 11, 2021
@heptaber heptaber requested a review from NikolaevArtem July 12, 2021 12:44
@heptaber heptaber added the readyForReview Sign for Artem to take a look label Jul 12, 2021
@NikolaevArtem NikolaevArtem added HW_1 HW_1 Good to go and removed bug Code fix needed homework_2 labels Jul 13, 2021
+ Using 2 functions that makes able to faster processing;
+ Using Math.abs for input value for being sure input number is positive (previous version was just stopping program execution by assert);
+ Assert now just checking input value is not zero;
- Current version using much more memory then previous;
@heptaber heptaber removed the readyForReview Sign for Artem to take a look label Jul 19, 2021
@NikolaevArtem NikolaevArtem removed the readyForReview Sign for Artem to take a look label Sep 21, 2021

public class Main {

public static void main(String[] args) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems indentation is shifted
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you describe this case in detail?
my view:
Screen Shot 2021-09-21 at 10 28 39 PM

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! The game interface looks pretty nice, even though it lacks row delimiters. Playing is comfortable, it shows pretty informative messages. Also, a lot of features are introduced, as well as exit code and description. You show good knowledge of Java Core and Java libraries, abstractions are mostly well-picked, architecture is good, but it can be improved. Also, you have lots of static, and it's not recommended.

Approved!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood you. Thanks for the review!


public void run() {
DialogueMenu.printGreetings();
DialogueMenu.printMainMenu();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name doesn't correlate with what it does

Copy link
Collaborator Author

@heptaber heptaber Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree, but i couldn't find better for this.
Before the game process started it's fully controlled by the menu - so User picks whatever wants, that's why it's like an entrance point into the game at this moment.


public final class ImmutableClass {

private final String name; // reference type
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's unclear in description, but I meant field which it mutable...

import static homework_4.custom_file_reader.utils.ModifiedStringPrinter.printModifiedString;


public class CustomFileReader {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good abstraction!

*/


public class Main {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite nice approach to HW_4! Looks good


private CustomRegexMatcher() { }

private static final String PHONE_NUMBER_FORMAT = "(\\+\\d)\\s\\(\\d{3}\\)\\s(\\d{3})-(\\d{4})";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: You could add some variability to regex (like, accept phone numbers with different country codes and etc)


@Override
public int hashCode() {
return new Random().nextInt(Integer.MAX_VALUE - 1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there;s no mutable key problem, it's just invalid hashcode implementation. Mutable key generates problem even if hashcode impl is valid

Copy link
Collaborator

@blowushka blowushka Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He fixed hashCode() and seems like mutable problem is appeared!

Comment on lines +8 to +9
private final int age;
private final String name;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opt: you could provide such fields and such lambda, that it'll make type conversion

Copy link
Owner

@NikolaevArtem NikolaevArtem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-done! You introduce new approaches to tasks, experiment with Java Core, which is cool. There are minor issues to fix, but in general, your HW looks really good! And special thanks for all code review efforts you did

@@ -48,7 +49,7 @@ public List<Number> getAllDataList() {
return new LinkedList<>(this.allDataList);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good approach! Approved!

@NikolaevArtem NikolaevArtem added HW_3 HW_6 and removed readyForReview Sign for Artem to take a look labels Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants